Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the Python execution pipeline from using raw Python and virtualenv to using uv for managing Python runtime and dependencies. The change introduces a new tower-uv crate that handles uv binary discovery/installation and simplifies the execution flow by leveraging uv's built-in dependency management.
Key Changes:
- Added
tower-uvcrate with automaticuvbinary discovery and installation capabilities - Removed virtualenv creation and pip dependency installation logic from
tower-runtime - Simplified Python program execution to use
uv runcommand instead of raw Python
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/tower-uv/src/lib.rs | Core uv wrapper with binary discovery and execution methods |
| crates/tower-uv/src/install.rs | Platform-specific uv binary download and installation logic |
| crates/tower-uv/Cargo.toml | Dependencies for the new tower-uv crate |
| crates/tower-runtime/src/local.rs | Updated to use uv instead of Python/pip/virtualenv |
| crates/tower-runtime/src/errors.rs | Added error conversion from tower-uv errors |
| crates/tower-runtime/Cargo.toml | Added tower-uv dependency |
| Cargo.toml | Workspace updates for new dependencies and crate |
crates/tower-uv/src/lib.rs
Outdated
| } else { | ||
| None | ||
| } | ||
| } else{ |
There was a problem hiding this comment.
Missing space before opening brace. Should be } else {
| } else{ | |
| } else { |
| } | ||
|
|
||
| // First, check if uv is already in the PATH | ||
| let output = Command::new("which") |
There was a problem hiding this comment.
Using which command is not portable across platforms (not available on Windows). Consider using a cross-platform solution or adding platform-specific logic.
crates/tower-uv/src/install.rs
Outdated
| let package_name = extract_package_name(archive.clone()); | ||
| Ok(path.join(package_name).join("uv")) |
There was a problem hiding this comment.
Inconsistent indentation - uses tabs instead of spaces. Should match the project's indentation style.
| let package_name = extract_package_name(archive.clone()); | |
| Ok(path.join(package_name).join("uv")) | |
| let package_name = extract_package_name(archive.clone()); | |
| Ok(path.join(package_name).join("uv")) |
crates/tower-uv/src/install.rs
Outdated
| let package_name = extract_package_name(archive.clone()); | ||
| Ok(path.join(package_name).join("uv")) |
There was a problem hiding this comment.
Inconsistent indentation - uses tabs instead of spaces. Should match the project's indentation style.
| let package_name = extract_package_name(archive.clone()); | |
| Ok(path.join(package_name).join("uv")) | |
| let package_name = extract_package_name(archive.clone()); | |
| Ok(path.join(package_name).join("uv")) |
crates/tower-uv/src/install.rs
Outdated
| std::fs::create_dir_all(&target_dir)?; | ||
|
|
||
| // Extract the file with proper error handling for compression | ||
| let filename = entries[entry_index].filename().as_str().unwrap_or("uv").to_string(); |
There was a problem hiding this comment.
Inconsistent indentation - uses tabs instead of spaces. Should match the project's indentation style.
| let filename = entries[entry_index].filename().as_str().unwrap_or("uv").to_string(); | |
| let filename = entries[entry_index].filename().as_str().unwrap_or("uv").to_string(); |
crates/tower-uv/src/install.rs
Outdated
|
|
||
| async fn download_uv_archive(path: &PathBuf, archive: String) -> Result<PathBuf, Error> { | ||
| debug!("Downloading UV archive: {}", archive); | ||
| let url = format!("https://github.com/astral-sh/uv/releases/download/0.7.13/{}", archive); |
There was a problem hiding this comment.
The UV version '0.7.13' is hardcoded. Consider making this configurable or using a constant to make updates easier.
| let url = format!("https://github.com/astral-sh/uv/releases/download/0.7.13/{}", archive); | |
| let url = format!("https://github.com/astral-sh/uv/releases/download/{}/{}", UV_VERSION, archive); |
| if let Some(version_part) = line.split_whitespace() | ||
| .find(|part| part.contains('.') && part.chars().next().unwrap_or('0').is_ascii_digit()) | ||
| { | ||
| let version_clean = version_part.trim_matches(|c: char| !c.is_ascii_digit() && c != '.'); | ||
| let parts: Vec<&str> = version_clean.split('.').collect(); | ||
|
|
||
| if parts.len() >= 2 { | ||
| if let (Ok(major), Ok(minor)) = (parts[0].parse::<u32>(), parts[1].parse::<u32>()) { | ||
| return major > req_major || (major == req_major && minor >= req_minor); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
[nitpick] Complex nested logic for version parsing. Consider extracting this into a separate helper function for better readability and testability.
| if let Some(version_part) = line.split_whitespace() | |
| .find(|part| part.contains('.') && part.chars().next().unwrap_or('0').is_ascii_digit()) | |
| { | |
| let version_clean = version_part.trim_matches(|c: char| !c.is_ascii_digit() && c != '.'); | |
| let parts: Vec<&str> = version_clean.split('.').collect(); | |
| if parts.len() >= 2 { | |
| if let (Ok(major), Ok(minor)) = (parts[0].parse::<u32>(), parts[1].parse::<u32>()) { | |
| return major > req_major || (major == req_major && minor >= req_minor); | |
| } | |
| } | |
| } | |
| } | |
| false | |
| } | |
| if let Some((major, minor)) = Self::extract_version_parts(line) { | |
| return major > req_major || (major == req_major && minor >= req_minor); | |
| } | |
| } | |
| false | |
| } | |
| /// Extract major and minor version numbers from a single line of the version string | |
| fn extract_version_parts(line: &str) -> Option<(u32, u32)> { | |
| if let Some(version_part) = line.split_whitespace() | |
| .find(|part| part.contains('.') && part.chars().next().unwrap_or('0').is_ascii_digit()) | |
| { | |
| let version_clean = version_part.trim_matches(|c: char| !c.is_ascii_digit() && c != '.'); | |
| let parts: Vec<&str> = version_clean.split('.').collect(); | |
| if parts.len() >= 2 { | |
| if let (Ok(major), Ok(minor)) = (parts[0].parse::<u32>(), parts[1].parse::<u32>()) { | |
| return Some((major, minor)); | |
| } | |
| } | |
| } | |
| None | |
| } |
There was a problem hiding this comment.
This needs to be rewritten eventually in general, so will skip it for now.
| } | ||
|
|
||
| fn make_env_vars(ctx: &tower_telemetry::Context, env: &str, cwd: &PathBuf, is_virtualenv: bool, secs: &HashMap<String, String>, params: &HashMap<String, String>, other_env_vars: &HashMap<String, String>) -> HashMap<String, String> { | ||
| fn make_env_vars(ctx: &tower_telemetry::Context, env: &str, cwd: &PathBuf, secs: &HashMap<String, String>, params: &HashMap<String, String>, other_env_vars: &HashMap<String, String>) -> HashMap<String, String> { |
There was a problem hiding this comment.
[nitpick] Function signature is very long with many parameters. Consider grouping related parameters into a struct for better maintainability.
There was a problem hiding this comment.
Likewise will ignore this for now, I don't think it's necessary.
konstantinoscs
left a comment
konstantinoscs
left a comment
There was a problem hiding this comment.
Can you run a build-tower-runner-binaries on this? I see changes to cargo.toml and I'm thinking that we should have some kind of way to safeguard against regressing from pure rustls...
|
|
||
| // If we're in a virtual environment, we need to add the bin directory to the PATH so that we | ||
| // can find any executables that were installed there. | ||
| if is_virtualenv { |
There was a problem hiding this comment.
So uv takes care of this?
|
Spoke with @konstantinoscs offline, we aren't able to product runner binaries with PRs in this repo because the relevant code needs to land in |
This PR migrates the python execution pipeline to use
uvinstead of raw Python everywhere. It has some code for making sureuvexists/runs on the relevant host, too.